Skip to content

TST: GH39443 - categorical datetime concat #41061

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

TST: GH39443 - categorical datetime concat #41061

wants to merge 2 commits into from

Conversation

arredond
Copy link
Contributor

@arredond arredond commented Apr 20, 2021 •
edited
Loading

@arredond arredond mentioned this pull request Apr 21, 2021
Closed
3 tasks
mzeitlin11
mzeitlin11 reviewed Apr 22, 2021
View reviewed changes
Copy link
Member

@mzeitlin11 mzeitlin11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pr @arredond! One comment, plus would want input from others on if a whatsnew is necessary here (since this specific pr doesn't fix the issue, but it was fixed silently in the previously backported #39615)

pandas/tests/reshape/concat/test_categorical.py Outdated
expected = DataFrame(
{
"x": Series(
[datetime(2021, 1, 1), datetime(2021, 1, 2)], dtype="category"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Referencing some of your exploration on the related issue, I think not returning a categorical on this concatenation is expected since the inputs don't have same categorical dtype (eg different categories).

The same behavior is seen if you just do something like replace the input to df1 and df2 with 1 and 2, respectively (gives int dtype result, not categorical).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes a lot of sense. I've rebased to my original commit removing the categorical dtype in the expected dataframe. However, I think the tests are going to fail again with seemingly contradictory messages.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on 1.1.5 the return type is datetime64[ns] and this is also the case on 1.2.4 and master. So testing that this matches the 1.1.5 behavior is fine.

@mzeitlin11 mzeitlin11 added Categorical Categorical Data Type Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels Apr 22, 2021
@arredond arredond force-pushed the categorical_datetime_concat_test branch from 37731b6 to 1295868 Compare April 27, 2021 12:51
@simonjayhawkins simonjayhawkins changed the title BUG: GH39443 - categorical datetime concat TST: GH39443 - categorical datetime concat May 25, 2021
@simonjayhawkins simonjayhawkins added the Testing pandas testing functions or related to the test suite label May 25, 2021
@simonjayhawkins
Copy link
Member

plus would want input from others on if a whatsnew is necessary here

not needed

@simonjayhawkins
Copy link
Member

@arredond can you merge master and ping on green

@arredond
Copy link
Contributor Author

arredond commented Jun 7, 2021

@simonjayhawkins merged but no green. Apart from a couple of conda build fails I'm back to my original error:

>       tm.assert_equal(result, expected)
E       AssertionError: Attributes of DataFrame.iloc[:, 0] (column name="x") are different
E       
E       Attribute "dtype" are different
E       [left]:  CategoricalDtype(categories=['2021-01-01', '2021-01-02'], ordered=False)
E       [right]: datetime64[ns]

@github-actions
Copy link
Contributor

github-actions bot commented Jul 8, 2021

This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this.

@github-actions github-actions bot added the Stale label Jul 8, 2021
@mroeschke
Copy link
Member

Thanks for the PR, but appears this PR has gone stale and looks like it needs a little further investigation to solve the bug. Closing, but please let us know if you'd be interested in continuing and we'd be happy to reopen.

@mroeschke mroeschke closed this Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonjayhawkins simonjayhawkins simonjayhawkins left review comments

@mzeitlin11 mzeitlin11 mzeitlin11 left review comments

Assignees
No one assigned
Labels
Categorical Categorical Data Type Reshaping Concat, Merge/Join, Stack/Unstack, Explode Stale Testing pandas testing functions or related to the test suite
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@arredond @simonjayhawkins @mroeschke @mzeitlin11